Skip to content

fix(rivetkit-napi): plumb ctx through serializeState TSF and stop panicking on runtime_state Ref drop#4725

Closed
NathanFlurry wants to merge 1 commit into04-23-chore_rivetkit_remove_legacy_metrics_prometheus_authfrom
04-23-fix_rivetkit-napi_plumb_ctx_through_serializestate_tsf_and_stop_panicking_on_runtime_state_ref_drop
Closed

fix(rivetkit-napi): plumb ctx through serializeState TSF and stop panicking on runtime_state Ref drop#4725
NathanFlurry wants to merge 1 commit into04-23-chore_rivetkit_remove_legacy_metrics_prometheus_authfrom
04-23-fix_rivetkit-napi_plumb_ctx_through_serializestate_tsf_and_stop_panicking_on_runtime_state_ref_drop

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Copy Markdown
Member Author

NathanFlurry commented Apr 24, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Code Review

PR: fix(rivetkit-napi): plumb ctx through serializeState TSF and stop panicking on runtime_state Ref drop
Status: Draft

Overview

This PR makes four targeted fixes in the NAPI bridge:

  1. Stops a debug-mode panic (and silent release-mode leak) when dropping a Ref<()> without an Env on a tokio worker thread.
  2. Adds a Drop impl on ActorContextShared with the same guard.
  3. Fixes the reason field in build_serialize_state_payload to use env.create_string_from_std instead of a raw String push.
  4. Removes the redundant name field from build_action_payload (the callback is already keyed by name at registration).
  5. Adds has_database propagation from buildActorConfig in TypeScript through to the NAPI JsActorConfig.
  6. Narrows SerializeStateReason in TypeScript to match the Rust enum ("save" | "inspector" only).

Critical Issue

has_database is added to From<JsActorConfig> for ActorConfigInput but ActorConfigInput in rivetkit-core does not have this field.

rivetkit-rust/packages/rivetkit-core/src/actor/config.rsActorConfigInput struct has no has_database field on either the base branch or the head branch of this PR. The From impl will fail to compile:

// actor_factory.rs — From<JsActorConfig> for ActorConfigInput
Self {
    name: value.name,
    icon: value.icon,
    has_database: value.has_database,  // ERROR: no field `has_database` on ActorConfigInput
    ...
}

Resolution options:

  • Add pub has_database: Option<bool> to ActorConfigInput and wire it through ActorConfig::from_input in rivetkit-core (likely the right path if the core needs to know about the database).
  • If has_database is only needed at the NAPI layer to decide whether to open SQLite, strip the field from the From<JsActorConfig> for ActorConfigInput conversion and keep it only in JsActorConfig.

mem::forget for Ref<()> (Medium)

The fix in reset_runtime_state and the new Drop impl is a pragmatic stopgap. The comment accurately describes the tradeoff: avoids a debug assert panic, but leaks one napi reference slot per actor wake cycle for the process lifetime.

if let Some(old) = self.runtime_state.lock().take() {
    std::mem::forget(old);  // intentional bounded leak
}

This is acceptable as a short-term fix, but a proper solution would schedule Ref::unref(env) back onto the Node.js event loop thread (e.g. via a ThreadsafeFunction with no payload, or a napi::Env-capturing closure queued through libuv). A follow-up task in .agent/todo/ or a TODO comment pointing to a tracking issue would be useful here so this isn't forgotten.


Correct Fixes

  • env.create_string_from_std(payload.reason) — using the explicit napi constructor instead of relying on blanket ToNapiValue for String is the right call for clarity and reliability.
  • Removing name from build_action_payload — actions are registered per-name, so the name is redundant in the JS-side payload. The field remains in ActionPayload and is used by TsfnPayloadSummary for logging, which is fine.
  • SerializeStateReason narrowing — aligns the TypeScript type alias with the Rust SerializeStateReason enum (Save/Inspector only). Correct.
  • hasDatabase: config.db !== undefined — clean detection of whether an SQLite database is configured; the boolean is derived from presence rather than re-checking at call time.

Minor

  • ActionPayload.name stays in the struct (consumed by TsfnPayloadSummary for logging) but is no longer forwarded to JS. No cleanup needed now, but worth noting in case the field becomes dead weight later.

Summary

The mem::forget approach and the other targeted fixes are sound. The blocking issue is that ActorConfigInput needs has_database added (or the field removed from the From conversion) before this will compile. Everything else looks correct and well-scoped.

@NathanFlurry NathanFlurry force-pushed the 04-23-fix_rivetkit-napi_plumb_ctx_through_serializestate_tsf_and_stop_panicking_on_runtime_state_ref_drop branch from e0c30f6 to 94270d3 Compare April 24, 2026 09:52
@NathanFlurry NathanFlurry mentioned this pull request Apr 24, 2026
11 tasks
@NathanFlurry NathanFlurry force-pushed the 04-23-chore_rivetkit_remove_legacy_metrics_prometheus_auth branch from 3a2efdf to d3104cb Compare April 24, 2026 10:19
@NathanFlurry NathanFlurry force-pushed the 04-23-fix_rivetkit-napi_plumb_ctx_through_serializestate_tsf_and_stop_panicking_on_runtime_state_ref_drop branch from 94270d3 to 9009f05 Compare April 24, 2026 10:19
@NathanFlurry NathanFlurry force-pushed the 04-23-chore_rivetkit_remove_legacy_metrics_prometheus_auth branch from d3104cb to c0106f8 Compare April 24, 2026 10:32
@NathanFlurry NathanFlurry force-pushed the 04-23-fix_rivetkit-napi_plumb_ctx_through_serializestate_tsf_and_stop_panicking_on_runtime_state_ref_drop branch from 9009f05 to 0e8d52d Compare April 24, 2026 10:32
@NathanFlurry NathanFlurry force-pushed the 04-23-chore_rivetkit_remove_legacy_metrics_prometheus_auth branch from c0106f8 to 441610e Compare April 24, 2026 11:48
@NathanFlurry NathanFlurry force-pushed the 04-23-fix_rivetkit-napi_plumb_ctx_through_serializestate_tsf_and_stop_panicking_on_runtime_state_ref_drop branch 2 times, most recently from 32b9abd to 8e8783c Compare April 24, 2026 12:14
@NathanFlurry NathanFlurry force-pushed the 04-23-fix_rivetkit-napi_plumb_ctx_through_serializestate_tsf_and_stop_panicking_on_runtime_state_ref_drop branch from 8e8783c to 81178fa Compare April 24, 2026 12:32
@NathanFlurry
Copy link
Copy Markdown
Member Author

Landed in main via stack-merge fast-forward push. Commits are in main; closing to match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant